[No QA] Fix useOnyx to skip state reset on initial mount#777
Conversation
| // subscribeToKey (e.g. addNullishStorageKey for skippable collection member ids) | ||
| // are reflected in the next getSnapshot. Resetting this flag does not change | ||
| // resultRef by itself, so it doesn't cause an extra mount render. | ||
| shouldGetCachedValueRef.current = true; |
There was a problem hiding this comment.
We are already setting shouldGetCachedValueRef.current = true; below, why do we need to set it again?
There was a problem hiding this comment.
Because we still need it for one edge case: skippable collection member IDs.
In OnyxUtils.subscribeToKey (lines 1065-1078), when the key matches a skippable ID, the function synchronously calls cache.addNullishStorageKey(mapping.key) and returns early without ever invoking the callback. That means connectionManager.connect's wrapping callback never fires for skippable keys, and the reset inside it never runs.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
I am closing this one to open by myself to be able to fulfull the checklist |
Details
Related Issues
Expensify/App#87850
Automated Tests
Manual Tests
Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari